[26.04_linux-nvidia] NVIDIA: SAUCE: watchdog: sbsa_gwdt: stop the watchdog across the whole system-sleep transition - #568

BaseOS Kernel Review

🔍 Review artifacts

• 🟠 Boro review: 1 finding
• 🔵 Test plan: ready for validation

📦 Kernel deb builds — 🟢 2/2 passed

• 🟢 amd64: build report and 4 debs
• 🟢 arm64: build report and 4 debs

This comment is maintained by BaseOS Reviewer and updated when the GitHub watcher publishes a newer review.

PR Validation Report

Patchscan ✅ No Missing Fixes

All cherry-picked commits checked — no missing upstream fixes found.

PR Lint ✅ All checks passed

Checking 1 commits...

Cherry-pick digest:
┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐
│ Local        │ Referenced upstream / Patch subject                              │ Patch-ID   │ Subject │ SoB chain                 │
├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤
│ 40aebd1a8a1f │ [SAUCE] watchdog: sbsa_gwdt: stop the watchdog across the whole  │ N/A        │ N/A     │ dcemin                    │
└──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘

Lint: all checks passed.

@dcemin-nv I created the Launchpad bug for this change:
https://bugs.launchpad.net/ubuntu/+source/linux-nvidia/+bug/2166302

Could you please add this link to the PR description?

Also we need below information to track these patches:
What are the upstream plans for these?
Are these related to a specific NVbug?

@dcemin-nv

• What are the upstream plans for this?
• Is this related to a specific NVbug?

Tracking info: internal NVbug 6611666 (watchdog initiated reset during suspend entry on the N1x stress runs). Upstream plan: the patch is prepared against current mainline and is being posted to linux-watchdog (Wim Van Sebroeck, Guenter Roeck cc linux-watchdog@vger.kernel.org); I will add the lore link here once it is archived.

This is what I found with Codex:
The watchdog still restarts during device resume, before PM_POST_. The PM notifier stops the hardware in PM_PREPARE and intends to restart it at PM_POST* (drivers/watchdog/sbsa_gwdt.c:304), but the existing device resume callback still calls sbsa_gwdt_start() whenever WDOG_HW_RUNNING is set (drivers/watchdog/sbsa_gwdt.c:463).
Since the notifier calls the driver stop directly, that bit stays set. Normal suspend runs dpm_resume_end() before PM_POST_SUSPEND (kernel/power/suspend.c:536, kernel/power/suspend.c:560), so a slow later resume/thaw path can still reset the system. The device resume path should skip restart when the notifier owns the sleep transition, or track a stopped_by_pm_notifier state and let only PM_POST_* re-arm.

@clsotog the finding is correct. The PM notifier stops the hardware by calling the driver's stop op directly, so WDOG_HW_RUNNING stays set, and the existing dev_pm_ops resume callback then restarts the watchdog during dpm_resume, before PM_POST_SUSPEND. That leaves the watchdog armed with nobody refreshing it through thaw, a shorter window than the entry side this patch fixes, but the same class of problem.

Fix for v2: with the notifier owning the whole sleep transition, the per device suspend and resume callbacks are redundant, so I will drop SET_SYSTEM_SLEEP_PM_OPS from the driver and let PM_PREPARE and PM_POST be the only stop and start points. I will also rebase the branch onto current 26.04_linux-nvidia, since it currently duplicates three commits that have since landed in the base. The same v2 goes to the linux-watchdog list.

Thanks one more little thing at the commit I see this line:
Change-Id: Id280d16321354d78f5dd766e82371bca25a4a2ea
We do not need this line for our commits to Canonical.

v2 pushed: e28c4a4 (569: 1f857bc). Fixes: 57d2caa trailer added, Change-Id line removed, description updated to the v2 behaviour (no dev_pm_ops, the notifier is the sole stop and start point). On the retest question: the change relative to v1 only removes the redundant device callbacks, so the v1 soak result is claimed for v2.

Thanks for the changes. There are comments in PR 569. This is what codex found in my part:

• P2 drivers/watchdog/sbsa_gwdt.c:322 removes the late device suspend stop, leaving a late-start window. PM_SUSPEND_PREPARE and hibernation prepare run before tasks are frozen, so userspace can still open/start /dev/watchdog after this notifier runs. Since the PR also removes dev_pm_ops, nothing stops that newly armed watchdog before device suspend/system sleep, and it can still reset the system during the transition. Keep a suspend-only device PM stop as a final guard, or otherwise block/re-stop starts while system sleep is in progress.

Thanks, the Fixes:/Change-Id cleanup and the v2 PR-description clarification are addressed on both paired heads. I found two additional items:

• The notifier can miss a concurrent probe. PM_SUSPEND_PREPARE runs at kernel/power/suspend.c:381, before dpm_prepare() waits for active probes and blocks new ones at drivers/base/power/main.c:2220-2227. A probe that passed the block check can therefore observe or start a running watchdog and reach notifier registration at drivers/watchdog/sbsa_gwdt.c:454-455 only after the prepare event has passed. Suspend then waits for the probe to finish, but with dev_pm_ops removed there is no remaining callback to stop that watchdog, so it can stay armed through the transition. Please make notifier readiness atomic with probe visibility/system-sleep entry, or retain a safe device-PM fallback that cannot re-arm early on resume.

• Both permanent commit messages still say that “with this patch applied and the watchdog force-enabled” the 198-suspend run had zero resets. The updated PR descriptions now clarify that the 198/200-cycle runs used v1 and these v2 heads were only compile-tested. Please scope the commit-message validation sentence to the v1-tested revision and state that v2 removes the device callbacks, so the durable history matches the actual validation provenance.

v3 pushed: 568 = 40aebd1, 569 = 3f6de47, same driver file on both. It addresses the four points raised by Nirmoy (Codex P1/P2), Cristian and Jamie:

• Restart racing a userspace stop or magic close after thaw (P1). The driver now keeps its own state, hw_armed and pm_stopped, under a spinlock shared by the watchdog ops and the sleep hooks. ops->stop clears both bits and stops the hardware; the PM_POST_* restart runs under the same lock and only if pm_stopped is still set and hw_armed still true, so a stop that landed after thaw wins and nothing re-arms behind the core's back. The stale WDOG_HW_RUNNING check is gone.
• Start after PREPARE and before task freezing (Cristian, and the second half of P1). ops->start sets hw_armed but, while pm_stopped is set, leaves the hardware stopped; the PM_POST_* hook arms it once everything has resumed. A watchdog opened during the entry window therefore cannot fire during the transition.
• Concurrent probe (Jamie). A suspend-only device callback remains as the final guard: it runs the same stop as the PREPARE hook, so a device whose probe overlapped the PREPARE event still gets stopped at dpm_suspend. It has no resume counterpart, so nothing re-arms the hardware before PM_POST_SUSPEND; the restart is owned by the notifier, which by then is registered.
• Notifier registration failure (P2). The notifier is registered first thing in probe, before early_enable can arm anything and before the watchdog device is registered, and its failure now fails the probe with dev_err_probe. Nothing is left armed on that path.

Commit message: the validation sentence is scoped to the first revision (notifier plus the original device callbacks, 198 suspends, zero resets) and states that this revision is compile tested; the Fixes: trailer is unchanged. Both flavours built with W=1 with no warnings, checkpatch strict is clean.

I agree with the current Boro finding about the remaining pm_stopped/late-start and concurrent-probe race, so I will not duplicate it.

Nit: Both PR descriptions still say “v2 is compile verified” and claim the v1 soak result for v2. These are now substantive v3 heads with new locking and a suspend fallback. Please update the validation paragraph to identify the exact v3 build validation and keep it distinct from the v1 runtime soak.